Repository navigation
Accept high-precision matrix storage in native splat sorting - #1894
bkaradzic-microsoft merged 6 commits into
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 3
Open (4)
std::quick_exit(1)will abort the entire test process (skipping normal teardown and… · New_mfrom aFloat32Arrayto anyObject, which also removes the previous… · New_mfrom aFloat32Arrayto anyObject, which also removes the previous… · NewNapi::Eval,Napi::Error). To avoid… · New
What changed in this PR
Adds matrix-storage compatibility to NativeOptimizations’ sortSplats so it can read Babylon.js model-view matrices stored as either Float32Array or ordinary JS number arrays (high-precision mode), and validates this behavior with a new unit test.
Changes:
- Read view-direction components from
modelView._mvia indexed object property access instead of assumingFloat32Array. - Add a regression unit test covering typed-array vs number-array matrices, both handedness settings, and multiple/single/empty splat inputs.
- Wire the new test into the UnitTests target and conditionally link NativeOptimizations when enabled.
| File | Description |
|---|---|
| Plugins/NativeOptimizations/Source/NativeOptimizations.cpp | Makes sortSplats accept both typed-array and number-array matrix storage by reading _m[2/6/10] via Get. |
| Apps/UnitTests/Source/Tests.NativeOptimizations.cpp | Adds a regression test ensuring matrix storage variations no longer throw and produce stable ordering. |
| Apps/UnitTests/CMakeLists.txt | Adds the new test source and conditionally links/enables NativeOptimizations for the UnitTests target. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 4
Open (4)
std::quick_exit(1)will terminate the entire test process immediately (potentially skipping other… · NewArrayis accepted, missing/non-numeric elements (e.g., holes/undefinedat indices… · NewArrayis accepted, missing/non-numeric elements (e.g., holes/undefinedat indices… · New
Resolved since last review (4)
_mfrom aFloat32Arrayto anyObject, which also removes the previous…_mfrom aFloat32Arrayto anyObject, which also removes the previous…std::quick_exit(1)will abort the entire test process (skipping normal teardown and…Napi::Eval,Napi::Error). To avoid…
|
Second-round Copilot follow-up is pushed as cc7cc3a: explicit checks for missing/non-numeric matrix coefficients, 18 regression cases, GoogleTest failure reporting before the worker-timeout hard exit, and an explanation of the algorithm-independent unique-depth ordering oracle. All four new threads are addressed and resolved. The red MacOS_Installation job on df90da7 is an infrastructure failure before source compilation: GitHub HTTPS connection timeouts while cloning bimg and bx, including the built-in retry. No compiler or unit-test failure was reported. The new head triggers fresh CI, so I am not also retrying the obsolete run or changing dependency pins/build logic to mask the network outage. Balanced re-review remains pending: the API previously accepted but ignored the effort override and launched Lite. I am not claiming a Balanced review or repeatedly requesting Lite as a substitute. |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 2
Open (2)
Resolved since last review (4)
Arrayis accepted, missing/non-numeric elements (e.g., holes/undefinedat indices…Arrayis accepted, missing/non-numeric elements (e.g., holes/undefinedat indices…std::quick_exit(1)will terminate the entire test process immediately (potentially skipping other…
cc7cc3a to
7ee8c2e
Compare
Review-Group: E4 Source: PR BabylonJS#1894 Squashed final review changes, including regressions and review follow-ups. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Read the three view-direction components through numeric properties so released Babylon.js high-precision matrix storage works alongside Float32Array. Keep the sorting algorithm and arithmetic unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Keep Float32Array and ordinary Array support while reporting an actionable TypeError for unsupported storage. Cover invalid inputs and explain why the runtime-worker timeout must avoid joining a stuck thread. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Validate the three matrix coefficients before numeric conversion and cover missing and nonnumeric elements. Report worker timeouts through GoogleTest before the necessary nonzero hard exit. Explain the unique-depth ordering oracle instead of duplicating the sorting implementation in the test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305
f720e88 to
09c9864
Compare
Summary
sortSplatsassumes the matrix's_mstorage is aFloat32Array, but released Babylon.js uses ordinary number arrays in high-precision matrix mode. Read the three numeric view-direction components through indexed object properties instead.This is matrix-storage compatibility, not a change to the sorting algorithm or arithmetic precision.
Independence and coordination
Based directly on upstream
masterat2a9dc944, with unchanged dependency pins and the published Babylon.js 9.21.2 contract. No protocol, engine-option, reference, or tolerance changes.#1883 independently replaces the sorting algorithm. This PR intentionally leaves that algorithm alone; please retain the storage-compatibility hunk when combining the two. It can land before or after that rewrite.
Validation
Float32Arrayand ordinary number-array matrices, both handedness settings, all three nonzero view-direction components, and empty/single/multiple splat inputs.Invalid argumenton the ordinary array; the extracted fix passes.